-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improvement: cancel current request in batched functions #5432
Conversation
ce3c761
to
bc0c04a
Compare
} | ||
.distinct | ||
.foreach { conn => | ||
conn.cancelCompilations() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was done on purpose since the cancelation was not properly propagated before
metals/src/main/scala/scala/meta/internal/metals/BatchedFunction.scala
Outdated
Show resolved
Hide resolved
if (!queue.isEmpty) { | ||
runAcquire() | ||
} | ||
} | ||
private def runAcquire(): Unit = { | ||
if (!isPaused.get() && lock.compareAndSet(false, true)) { | ||
runRelease() | ||
val id = rand.nextInt() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random could be zero,no? This should probably be a raising number. Why do we need the additional int though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before cancel()
would clear the queue and do unlock()
. But we still may get a response from the request that was currently processed, which would also do ulock()
as callback. We are loosing control over the lock a bit. Not sure if that is a real problem though. Now when we also do cancel on the currently processed request, we have no guarantee the futures underneath will actually react to cancel
, so I'm making sure that only the non-cancelled currently processed request will have onComplete
executed. There is most probably a better way to do this.
40c1b48
to
4679686
Compare
d0d21fe
to
956a014
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I think this is almost ready, though I have doubts if the tests wouldn't cause us any flakiness
metals/src/main/scala/scala/meta/internal/metals/BatchedFunction.scala
Outdated
Show resolved
Hide resolved
) | ||
.zip { | ||
// wait until the compilation start | ||
Thread.sleep(1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this have a potential to be flaky? A compilation might take less than that, no? You could try some macros that do Thread.sleep to make sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
956a014
to
af79a8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Could you just rebase on main?
af79a8e
to
b1e86f0
Compare
No description provided.